[USP][QA] Gate the Dart/Wasm boundary and restore WebSocket exports - #1156
[USP][QA] Gate the Dart/Wasm boundary and restore WebSocket exports#1156DevenDucommun wants to merge 7 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
AustinChangLinksys
left a comment
There was a problem hiding this comment.
🤖 Automated Review — Round 1 · 5beff30..dcecf8b (full)
Verdict: APPROVE — No Critical issues; boundary gate is architecturally sound and all 9 tests pass. Several tooling-robustness Warnings worth addressing before the gate is broadly relied on in CI.
| Conf. | Where | Issue (one-liner) | |
|---|---|---|---|
| Warning | High | usp_ws_client_wrapper.dart:163 |
[both reviewers] connect() polling force-promotes connecting→open after 1 s; half-open socket silently returned to callers |
| Warning | High | check_boundary.py:324 |
List-valued intentionally_unbound entry crashes gate with AttributeError instead of clean FAIL message |
| Warning | High | check_boundary.py:276 |
External getter/setter declarations silently excluded from boundary coverage (filter requires \() |
| Warning | High | usp-boundary.yml:17 |
actions/checkout@v4 unpinned — mutable tag defeats the integrity-gate's purpose |
| Warning | High | usp_ws_client_wrapper.dart:56-62 |
Retained JS shims lack doc comment explaining why they were not replaced with direct Dart interop |
| Warning | High | doc/usp/vendored-artifacts.md |
No note that subscribe/unsubscribe were intentionally unbound (future auditors cannot distinguish from silently-broken binding) |
| Warning | Med | check_boundary.py:232 |
Parameter.optional flag populated but never compared; optional/required agreement not actually verified |
| Warning | Med | test_boundary.py |
No negative test for missing required_dart_classes; floor-enforcement code path uncovered |
| Suggestion | High | usp-boundary.yml:19 |
--verify-upstream omitted; step name misleadingly says "recorded source reference" but only checks local hashes |
| Suggestion | High | check_boundary.py:21 + usp-boundary.yml |
No Python version pin; tuple[...] syntax (PEP 585) fragile on pre-3.9 runners |
| Suggestion | High | analysis_options.yaml:9 |
Minor: exclude: placed before errors: (Flutter convention is errors: first) |
| Suggestion | Med | check_boundary.py:198 |
UspWsClient/UspWsClientJS missing from normalized_type mapping; latent false positive if future method returns UspWsClient |
Confidence: High = code-verified · Med = located + reasoned, not fully confirmed · Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.
Warning Details
W-1 · usp_ws_client_wrapper.dart:163 — WebSocket connect polling force-promotes state [both reviewers] (High confidence)
Both reviewers independently flagged: after 10 x 100 ms the wrapper silently sets _currentState = WsConnectionState.open regardless of actual socket state:
// usp_ws_client_wrapper.dart ~line 163
wrapper._currentState = WsConnectionState.open;
logger.i('$_tag Connected to $url (no open callback, assuming success)');
return wrapper;If the WebSocket handshake takes more than 1 s (slow LAN, congested path), connect() returns a half-open UspWsClientWrapper. Callers pass the _currentState == open guard but the underlying socket may not be ready — the first USP frames can be silently lost. This is pre-existing code, but this PR restores and re-exports this path via the new Wasm bindings, making the risk surface larger.
Fix: Tie the completer to the JS onStateChange callback (the WASM side fires this reliably). If the polling fallback must be kept for old-browser compatibility, throw StateError rather than silently assuming open.
W-2 · check_boundary.py:324 — List-valued intentionally_unbound crashes gate with AttributeError (High confidence)
# check_boundary.py line ~305 + ~324
unbound = policy.get("intentionally_unbound", {})
reason = unbound.get(class_name, {}).get(method_name)
# ^^^^ AttributeError if value is a listIf a contributor writes "UspClient": ["subscribe", "unsubscribe"] (the natural intuition) instead of {"subscribe": "reason string"}, the .get(method_name) on a list raises AttributeError. This exception is not in the except (OSError, ValueError, json.JSONDecodeError) catch at main(), so the gate aborts with a traceback rather than a clean FAIL message.
Fix: Validate at schema load time that each class value under intentionally_unbound is a dict. Add a test fixture with the wrong shape and assert a clean FAIL.
W-3 · check_boundary.py:276 — External getter declarations silently excluded (High confidence)
# check_boundary.py line 276
if re.search(r"\bexternal\b[^;]*\(", text, flags=re.DOTALL):
discovered.append(path)The filter requires \( — external getters/setters have no parentheses and are permanently invisible. Example from usp_wasm_init_web.dart:
@JS('__uspClientReady')
external JSPromise<JSBoolean>? get _uspClientReady; // excluded by discovery filterThis file is silently skipped; the recognized != declared integrity count is never reached for it. Any future @JS() external getter added under lib/core/usp/web/ will be invisible to the boundary gate.
Fix: Widen the discovery filter to r"\bexternal\b" and extend parse_dart_globals to handle get/set accessors. Add a fixture test.
W-4 · usp-boundary.yml:17 — Unpinned actions/checkout (High confidence)
- uses: actions/checkout@v4 # mutable tagThe new workflow's purpose is to verify integrity of vendored binary artifacts. Using a mutable action tag means a supply-chain-compromised version of checkout could tamper with the workspace before the hash check runs, defeating the gate's purpose.
Fix: Pin to a commit SHA: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
W-5 · usp_ws_client_wrapper.dart:56-62 — Retained JS shims lack documentation (High confidence)
After removing _uspWsSendRecord (the explicit debug shim), two JS shims remain without explanation:
@JS('sendWebSocketConnectNative')
external JSPromise<JSAny?> _sendWebSocketConnectNative(...);
@JS('sendOperateRecordNative')
external JSPromise<JSAny?> _sendOperateRecordNative(...);A future developer seeing sendRecord_() now using direct Dart interop while these two shims have no comment may delete them as dead code.
Fix: Add a block comment explaining the reason the shims are retained (WASM linear memory detach: wasm-bindgen returns a Uint8Array backed by WASM linear memory; during a malloc, memory can grow and detach the ArrayBuffer; the native JS shim copies to JS heap first).
W-6 · doc/usp/vendored-artifacts.md — No note on subscribe/unsubscribe removal rationale (High confidence)
subscribe (3-arg in v0.12.0 TypeScript) and unsubscribe existed in the Wasm API and were never correctly bound in Dart (old binding used 1 arg). They are now listed as intentionally_unbound in policy.json, but no human-readable note exists in the docs. Without it, a future auditor cannot distinguish "intentionally never bound" from "was bound at wrong arity and silently broken."
Fix: Add one sentence to doc/usp/vendored-artifacts.md: "Note: UspClient.subscribe (3-arg form in v0.12.0) and unsubscribe are intentionally not bound; the app uses UspBridgeClient + SSE for push notifications instead."
W-7 · check_boundary.py:232 — optional flag populated but never compared (Med confidence)
for index, (dart_param, ts_param) in enumerate(zip(dart.parameters, typescript.parameters), start=1):
if normalized_type(dart_param.type_name) != normalized_type(ts_param.type_name):
errors.append(...)
# Parameter.optional is populated by ts_parameters() but never consultedNo current signature mismatch exists, but the gate gives false confidence that optional/required agreement is verified. The TypeScript refreshToken(token?: string | null) is optional; the Dart binding requires positional (even if null).
Fix: Add if dart_param.optional != ts_param.optional: errors.append(...) after the type check, or add a comment explicitly stating optionality comparison is out-of-scope.
W-8 · test_boundary.py — No negative test for required_dart_classes (Med confidence)
The floor-enforcement path (required_dart_classes in policy.json) is tested only positively. No test asserts "required class absent → FAIL." If policy.json is accidentally cleared or a class renamed, the gate silently passes.
Fix:
def test_missing_required_class_fails(self):
policy = {"schema_version": 1, "minimum_decisions": 1,
"required_dart_classes": ["MissingClass"],
"intentionally_unbound": {}, "local_js_globals": {}}
_, errors = run_check(dts, [], policy, temp)
self.assertTrue(any("required Dart JS class" in e for e in errors), errors)Suggestion Details
S-1 · usp-boundary.yml:19 — --verify-upstream omitted; misleading step name (High confidence)
The step is named "Verify vendored artifact hashes and recorded source reference" but only local SHA-256 hashes are checked. The upstream commit cross-check is never run in CI. The README.md correctly discloses this limitation, but the step name contradicts it.
Fix: Rename to "Verify vendored artifact hashes (local only)" and add comment # --verify-upstream omitted: producer repo owns the cross-repo rebuild gate.
S-2 · No Python version pin; tuple[...] syntax requires Python 3.9+ (High confidence)
check_boundary.py:21 uses tuple[Parameter, ...] (PEP 585) without from __future__ import annotations. ubuntu-latest currently ships Python 3.10+ so this works today, but is fragile against runner image regression.
Fix: Add actions/setup-python@v5 with python-version: "3.11" to the workflow.
S-3 · analysis_options.yaml:9 — Minor ordering (High confidence)
exclude: is placed before errors: under analyzer:. Flutter convention lists errors: first. No functional impact.
S-4 · check_boundary.py:198 — UspWsClient/UspWsClientJS absent from normalized_type (Med confidence)
UspClient/UspClientBuilderJS variants are mapped but UspWsClient/UspWsClientJS are not. Currently safe because UspWsClientJS only appears in local_js_globals (arity-only check). If a future method returns UspWsClient, raw string comparison produces a false-positive type-mismatch error.
Fix: Add "UspWsClient": "usp-ws-client", "UspWsClientJS": "usp-ws-client" to the mapping dict.
What looks good
- Core boundary gate design: Correctly fails closed when any
externaldeclaration is unrecognized;minimum_decisions: 40floor prevents vacuous passes. Gate runs cleanly:PASS: 40 boundary decisions verified. - All 9 unit tests pass (
Ran 9 tests in 0.031s — OK), including the arity-mismatch fixture that reproduces the original 1-vs-3 arg defect. - subscribe/unsubscribe removal: Stub and web impl correctly symmetric; policy correctly records the architectural reason.
_uspWsSendRecorddebug export removed: No global JS debug surface remains;sendRecordnow correctly uses_jsClient.sendRecord_().- GitHub Actions permissions:
contents: readonly — correct minimal scope. - SHA-256 hash verification:
hashlib.sha256used correctly; all three vendored artifacts verified against manifest. - Dart analyzer fixture exclusion:
tools/usp_boundary/fixtures/**correctly excluded to prevent analyzer errors from intentionally invalid test fixtures. - Architecture layering: No violations of three-layer dependency rule.
FirmwareWsUploadStrategyis correctly in the Service layer; mutation lock held by the callingFirmwareUpdateNotifier(confirmed atfirmware_update_notifier.dart:260,277). getTokenbinding retained: Correctly kept despite other removals from the extension type.UspWsClientFinalizationregistry: Correctly added alongsideUspClientFinalizationandUspClientBuilderFinalization— no memory leak for the newUspWsClientclass.
Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.
Base branch is stale — should target
|
| Branch | Last commit | Note |
|---|---|---|
dev-2.7.0 |
2026-07-16 06:59:46Z | newest branch in the repo — current 2.x integration line |
usp (this PR's base) |
2026-07-16 06:23:26Z | ~36 min behind dev-2.7.0 |
dev-2.6.0 |
2026-07-16 05:42:11Z | older |
The existence of a merge/dev-2.6.0-to-usp branch confirms usp is a side integration branch that has to be manually caught up from the dev-* line — i.e. it lags mainline. This PR currently merges into that lagging side branch rather than the newest dev line.
Suggestion: retarget the base to dev-2.7.0.
⚠️ One coupling to handle if you retarget
The boundary gate this PR introduces is pinned to the usp branch:
.github/workflows/usp-boundary.ymltriggers only on PRs/pushes targetinguspci.ymlwiresuspPRs into the analyze/test/release-web-build flow
If the base moves to dev-2.7.0 without updating those trigger conditions, the gate silently stops running — which defeats the purpose of the PR. So retargeting must be done together with updating the workflow branches: filters (to dev-2.7.0, or a broader pattern covering the active dev line).
Not touching the PR state here — flagging for your call on (a) the retarget and (b) the paired workflow trigger update.
dcecf8b to
3a7363d
Compare
|
@AustinChangLinksys I pushed a rebased follow-up addressing the review findings:
Current evidence: 41 boundary decisions, 12 checker tests, 3,329 PrivacyGUI unit tests, both GitHub checks green. The PR remains blocked from merge until producer #44/#45 land and its manifest is refreshed to #45's resulting mainline SHA. Please re-review commit |
Outcome
Closes #1153 by making the vendored TypeScript declaration a checked consumer
contract, fixing the WebSocket readiness path exposed by review, and failing
firmware upload preparation closed when the USP WebSocket handshake does not
complete.
This branch is rebased onto
dev-2.7.0.Merge dependency
This PR depends on:
The current manifest records green producer PR head
b5e65ae9ce3fc5d61abe63adb269d77a5a7a9cbdand its exact CI-built package.Merge #44 first, retarget and merge #45 to
main, then refresh this PR to #45'sresulting mainline commit before merging.
Runtime corrections
subscribe()/unsubscribe()Wasmwrapper; PrivacyGUI subscriptions remain on
UspBridgeClientplusauthenticated SSE
undefined debug send shim with the real
UspWsClient.sendRecord()bindingconnect()Promise resolve only after the browser reachesWebSocket
OPEN, rejecting failed upgradesresources, and fall back to the HTTP strategy
Boundary gate
externalbelowlib/core/usp/web, including top-levelgetters such as
__uspClientReadylocal-JavaScript shims/values
policy shapes, reasons for intentionally unbound methods, and no stale policy
entries
cross-repository provenance
uspordev-*Parameter optionality is intentionally not claimed as syntax-equivalent to Dart
nullability; optionality-sensitive behavior requires wrapper tests.
Verification
Evidence boundary
This proves the producer/consumer API, local artifact integrity, controlled
WebSocket upgrade behavior, and consumer failure/fallback behavior. It does not
claim a successful browser upload against a router, QA-lab qualification, or
TR-369 conformance.
No ownership assignment is made by this PR.